Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

allow using any stage as a startup stage #2248

Closed
wants to merge 1 commit into from
Closed

allow using any stage as a startup stage #2248

wants to merge 1 commit into from

Conversation

mockersf
Copy link
Member

Fixes #2246, #2229

  • methods add_stage, add_stage_before and add_stage_after also adds an empty stage with the same label to the startup schedule
  • all CoreStage are added to the startup schedule
  • StartupStages now work as aliases of matching CoreStages (Startup -> Update, PreStartup -> PreUpdate, PostStartup -> PostUpdate)

This means:

  • for the default use case, no more surprising differences between startup and normal stages
  • extra stages are added to the startup schedule, but it shouldn't impact much as they'll remain empty and only run once
  • for the advanced user, nothing changed and they can still add custom stages with the explicit StartupStage

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels May 24, 2021
@alice-i-cecile
Copy link
Member

This is a bit magical; I'm not sure how I feel about doing all of this implicitly. It will solve the new user confusion there, but I'm a bit nervous about obscuring the correct mental model.

@cart
Copy link
Member

cart commented May 24, 2021

Yup I think it makes the system much harder to reason about. I think we should consider alternatives, such as:

  1. Just hard-code a better error message for this specific case (not a huge fan of this, but at least it is scoped)
  2. Remove the "subschedule" in favor of separate PreStartup/Startup/PostStartup stages in the main schedule (each with their own runonce run criteria).
  3. Other
  4. Ideas
  5. Here 😄

@mockersf
Copy link
Member Author

This is a bit magical

But it was fun magic to write!

It completely abuses the dynamic traits we have though.

Ratysz
Ratysz reviewed May 25, 2021
View reviewed changes
Copy link
Contributor

@Ratysz Ratysz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation does look like abuse on a level even greater than the dyn hack itself 😄

Both of the linked problems are, in essence, user errors. I don't think we need any new kind of automagic here: what we have now makes perfect sense once you realise what's going on (i.e. that the startup stage contains the startup schedule with its own stages). So, we just need better docs and a specialized, more descriptive error.

@Ratysz
Copy link
Contributor

Ratysz commented May 25, 2021

Another idea: pub trait StartupStageLabel.

@cart
Copy link
Member

cart commented May 25, 2021

Another idea: pub trait StartupStageLabel.

This solves this specific problem, but it would require hard-coding "startup stage labels" into Schedule right? Not sure I want to compromise a "generic" api for a specific use case.

@Ratysz
Copy link
Contributor

Ratysz commented May 25, 2021

hard-coding

Oh. I missed that. Yeah, that idea just went from "ehh" to "no way" in my head.

@alice-i-cecile
Copy link
Member

  1. Remove the "subschedule" in favor of separate PreStartup/Startup/PostStartup stages in the main schedule (each with their own runonce run criteria).

I like this a lot as a solution; I think it's much clearer for beginners and doesn't involve any trickery. I don't see any good occasions where end users will want to refer to all startup stages at once in a way that the nested stages encourage.

@mockersf mockersf marked this pull request as draft June 21, 2021 23:19
@cart cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021
@mockersf mockersf removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 24, 2021
@alice-i-cecile
Copy link
Member

I think the consensus on this is pretty 👎🏽 @mockersf <3 Going to close this out and we can look at this problem in #2801's schedule overhaul.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ratysz Ratysz Ratysz left review comments

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

add_startup_system_to_stage is broken
4 participants
@mockersf @alice-i-cecile @cart @Ratysz